Skip to content
This repository has been archived by the owner on Jun 20, 2024. It is now read-only.

Use alloy instead of primitive-types and ethereum-types #78

Merged
merged 13 commits into from
May 28, 2024

Conversation

0xaatif
Copy link
Contributor

@0xaatif 0xaatif commented May 23, 2024

obsoletes 0xPolygonZero/eth-tx-proof#22 as per 0xPolygonZero/zk_evm#157 (comment)

  • rewrite ::rpc
  • add ::rpc::Compat
  • trim deps

Reviewers

  • this is the most risky change:

    zero-bin/rpc/src/lib.rs

    Lines 172 to 185 in 3a5b21b

    let alloy::primitives::Bloom(alloy::primitives::FixedBytes(src)) = self;
    // have u8 * 256
    // want U256 * 8
    // (no unsafe, no unstable)
    let mut chunks = src.chunks_exact(32);
    let dst = core::array::from_fn(|_ix| {
    // This is a bit spicy because we're going from an uninterpeted array of bytes
    // to wide integers, but we trust this `From` impl to do the right thing
    __compat_primitive_types::U256::from(
    <[u8; 32]>::try_from(chunks.next().unwrap()).unwrap(),
    )
    });
    assert_eq!(chunks.len(), 0);
    dst
  • Unsure how to test this change at all, since this branch and the target branch both fail CI in the same way:

@atanmarko atanmarko self-requested a review May 24, 2024 11:09
keccak-hash = "0.10.0"
alloy = { git = "https://github.com/alloy-rs/alloy", features = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does anyone happen to know why they haven't/aren't releasing this? It makes me slightly uncomfortable with unreleased code but I don't think we really have another option here. We've been looking at alloy in the agglayer too but haven't made the switch yet for this exact reason. But... I think we just bite the bullet here.

rpc/src/main.rs Outdated Show resolved Hide resolved
rpc/src/main.rs Outdated Show resolved Hide resolved
rpc/src/main.rs Outdated Show resolved Hide resolved
rpc/src/lib.rs Outdated Show resolved Hide resolved
rpc/src/lib.rs Show resolved Hide resolved
}

#[test]
fn bloom() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to gently push for more tests here, I guess that's not a massive surprise :)

rpc/src/lib.rs Show resolved Hide resolved
Copy link
Collaborator

@Nashtare Nashtare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not much to add, looks good to me overall, thanks!

rpc/src/lib.rs Show resolved Hide resolved
rpc/src/lib.rs Show resolved Hide resolved
rpc/src/lib.rs Show resolved Hide resolved
@0xaatif 0xaatif merged commit a95adf9 into develop May 28, 2024
3 checks passed
@0xaatif 0xaatif deleted the 0xaatif/ethers2alloy branch May 28, 2024 16:15
@Nashtare
Copy link
Collaborator

@0xaatif you mentioned having trouble testing this branch because of some failure. Has this been resolved since?

@muursh
Copy link
Contributor

muursh commented May 28, 2024

@0xaatif you mentioned having trouble testing this branch because of some failure. Has this been resolved since?

#79

@Nashtare
Copy link
Collaborator

I think the error quoted in the CI is independent of the linker error. It was complaining about Error: invalid type: integer 2024, expected struct ProofWithPublicInputs at line 1 column 5

@cpubot
Copy link
Contributor

cpubot commented May 28, 2024

I think the error quoted in the CI is independent of the linker error. It was complaining about Error: invalid type: integer 2024, expected struct ProofWithPublicInputs at line 1 column 5

I believe so as well. Resolving the linker error resolved the compile error, but the test returns this error now. Not sure how long this has been broken, but seems unrelated

@muursh
Copy link
Contributor

muursh commented May 28, 2024

I mean I'm open to believing it really exists if someone can actually make it happen

@Nashtare
Copy link
Collaborator

Not sure how long this has been broken, but seems unrelated

I don't know how this failure is reproducible, the script is passing fine on my laptop.
However, I think this branch hasn't been tested properly before merging. Compilation with the test_only feature is broken.

@muursh
Copy link
Contributor

muursh commented May 28, 2024

Compilation with the test_only feature is broken.

@0xaatif

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants